-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RectangleSelectionTool #162
base: main
Are you sure you want to change the base?
Conversation
from chaco.abstract_overlay import AbstractOverlay | ||
|
||
|
||
class RectangleSelectionTool(AbstractOverlay, BaseTool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know there should be not need to subclass from BaseTool
in order for the RectangleSelection
to work as a tool. The tools
attribute is defined in Interactor
and there is no constraint (If there was then it would probably be Interactor
which all overlays/components have as parent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, if we use this class as a tool it will not draw itself since tools only receive key and mouse events, but do not participate by default in drawing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inheritance is copied from BetterSelectingZoom
, which subclasses AbstractOverlay
and BetterZoom
, which itself inherits from BaseTool
and ToolHistoryMixin
.
Would it be better to separate the tool and the overlay, as in RangeSelection
and RangeSelectionOverlay
?
I would be good to have basic testing for the new overlay (see example using |
x2, y2 = self._screen_end + self._move_offset | ||
rect = (x, y, x2 - x + 1, y2 - y + 1) | ||
if self.color != "transparent": | ||
if self.alpha: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would place this logic into a cached property (something named fill_color
). It would make the code easier to understand.
…ctingZoom as was done for the rest of this tool.
Completing this PR probably needs to wait for a discussion on the use case and the canonical architecture for tools and overlays. |
I'm currently working on building a tool to crop regions of an image, and this overlay has proved very useful. We don't necessarily need to merge it because of my use case, but it should probably be documented that someone else has found this useful since it was originally written. |
Is this PR superseded by #482 ? |
They are different enough that this probably shouldn't be closed - this looks like it is designed for selecting rectangular regions of image plots, not scatter plots. |
…fault image for the demo
… into rectangle_selection_tool
It sounds like this tool could use a name change to highlight the difference from |
This PR adds a RectangleSelectionTool based on the BetterSelectingZoom tool (thanks @tonysyu for the initial implementation). As such, it includes an
overlay()
method and subclassesAbstractOverlay
as well asBaseTool
so that it can be appended both to the tools and overlays lists.Included is a demo
chaco/examples/demos/basic/rectangle_selection.py
which uses the rectangle tool to select a region in one plot to display zoomed in the other plot.